Skip to content

feat(auth): implement MCP auth tool-level scopes validation#3049

Open
duwenxin99 wants to merge 4 commits intofeat/refactor-tool-interfacefrom
tool-auth
Open

feat(auth): implement MCP auth tool-level scopes validation#3049
duwenxin99 wants to merge 4 commits intofeat/refactor-tool-interfacefrom
tool-auth

Conversation

@duwenxin99
Copy link
Copy Markdown
Contributor

No description provided.

@duwenxin99 duwenxin99 requested review from a team as code owners April 13, 2026 22:46
@duwenxin99 duwenxin99 changed the base branch from main to feat/refactor-tool-interface April 13, 2026 22:47
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements tool-level scope authorization for the Model Context Protocol (MCP), enabling granular access control for database tools. The changes include updating the Tool interface, adding scopesRequired configuration to all supported database tools, and integrating scope verification logic into the MCP request handler. Feedback focuses on resolving critical field shadowing in Serverless Spark configurations and eliminating redundant global scope registries in favor of the new interface methods. Additionally, improvements were suggested for more robust scope string parsing and ensuring proper header value escaping using the %q verb.

I am having trouble creating individual review comments. Click here to see my feedback.

internal/tools/serverlessspark/serverlesssparkcreatepysparkbatch/serverlesssparkcreatepysparkbatch.go (46-49)

high

The ScopesRequired field is already present in the embedded createbatch.Config struct. Adding it again here shadows the base field. When the YAML is decoded, the outer field will be populated while the inner one (used by the embedded tool logic) will remain empty, effectively breaking scope enforcement for this tool.

	createbatch.Config
}

internal/tools/serverlessspark/serverlesssparkcreatesparkbatch/serverlesssparkcreatesparkbatch.go (46-49)

high

The ScopesRequired field is already present in the embedded createbatch.Config struct. Adding it again here shadows the base field, which will cause the tool to ignore configured scopes.

	createbatch.Config
}

internal/server/config.go (319-332)

medium

This manual parsing of scopesRequired and its storage in a global map is redundant. The field is already automatically unmarshaled into the tool's Config struct by the factory (as seen in the changes to individual tool files). Since the Tool interface now includes a GetScopesRequired() method, it is better to rely on the tool instance itself to provide its required scopes.

To maintain consistency with authRequired, you can simply ensure the field is initialized if missing.

	// Ensure scopesRequired is initialized if not present
	if _, ok := r["scopesRequired"]; !ok {
		r["scopesRequired"] = []string{}
	}

internal/server/mcp.go (585)

medium

The error_description value should be properly escaped to ensure the header remains valid if the error message contains double quotes. Using the %q verb in fmt.Sprintf will automatically handle quoting and escaping of the string.

					w.Header().Set("WWW-Authenticate", fmt.Sprintf("Bearer error=\"insufficient_scope\", scope=\"%s\", resource_metadata=\"%s\", error_description=%q", strings.Join(mcpErr.ScopesRequired, " "), s.toolboxUrl+"/.well-known/oauth-protected-resource", mcpErr.Message))

internal/server/mcp/v20251125/method.go (213)

medium

Instead of using the global tools.GetToolScopes map, use the GetScopesRequired() method from the Tool interface. This makes the tool implementation self-contained and avoids the need for a global registry of scopes.

	toolScopes := tool.GetScopesRequired()

internal/server/mcp/v20251125/method.go (226)

medium

Using strings.Fields is more robust than strings.Split with a single space, as it correctly handles multiple spaces and returns an empty slice for empty strings, avoiding potential issues with empty string elements in the resulting slice.

		tokenScopes := strings.Fields(scopeClaim)

internal/tools/tools.go (38-48)

medium

The toolScopes map and its associated getter/setter functions are redundant now that the Tool interface includes the GetScopesRequired() method and the scopes are being unmarshaled directly into the tool configurations.

@duwenxin99 duwenxin99 changed the title feat: implement MCP tool-level scopes and auth challenge feat(auth): implement MCP auth tool-level scopes validation Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants